Hmm. This seems like one of those things it would be really helpful to have a test case for.

How should I review? Do we need to accommodate type=radio as well?

@danielbachhuber No, I don't think we need to accommodate the radio button.

What we've changed here is how a truthy default works with the checkbox.

Before this fix, assuming prop is a checkbox attribute with a truthy default, if the user unchecked prop then there'd be a loss of information as [shortcode] would be passed back to TinyMCE. Then when the checkbox field was rendered again, the default would again override the false value for the field.

If the default of a radio is some 'value', then we expect the shortcode to look like [shortcode prop="value"] when it's passed back to the editor. This value would be reflected in the input and all would be good. If no default value is provided then the functional default is no selection, which would just mean that the attribute doesn't get passed to the shortcode, but this is fine as the input will render with no selection anyway.

I've added a test based on @niallkennedy's use case.

These gifs aren't good quality but they demonstrate how this PR improves the use case.

Before

After

@davisshaver I think we should chat about this some during the Shortcake chat today. I'm not 100% sure this is the behavior we want.

@danielbachhuber Sure, fine with me.

Maybe we could ask @niallkennedy what he expects, too.

I look for falsey values (0, 'false', etc) for a shortcode override. I ignore empty string values for a boolean attribute.

The checked behavior passes a 'true' string to be interpreted by the shortcode handler. If you support Shortcake you also need to accept a value of 'true' for a boolean shortcode attribute. A string of 'false' represents the opposite state in my mind, and provides consistency between the required shortcode attribute values a shortcode handler should accept to be compatible with Shortcake.

Thanks @niallkennedy!

@danielbachhuber based on #413 (comment), what do you think about merging? Should we get a second opinion from @mattheu or @goldenapples?

@danielbachhuber Assigning this to you for a second opinion – still hesitant about behavior change?

still hesitant about behavior change?

Yes. For now, let's just bump it from the v0.5.0 milestone.

It would be great to see this merged at some point. And if not, at least an explanation of why this might be unwanted behavior and possible workarounds? Currently I'm just avoiding checkbox fields because of this and using select fields instead.

I like this concept, but it seems that we would need some way of enabling this as an opt-in feature on an attribute. I know that a number of shortcodes I've built would break if this were merged as-is, as there are plenty of places where I just check for a boolean attribute to be present in the callback, and attribute="false" would evaluate to true in the callback.

It might be good to follow the pattern we established with the "encoded" parameter, where a flag set on an attribute could be used in a filter on "shortcode_atts_{$shortcode_tag}" like this one is to parse boolean attributes and either unset the attribute or set it to a boolean false rather than a string.